Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added querystring search get option #4658

Merged
merged 4 commits into from
Apr 6, 2023
Merged

Conversation

robgietema
Copy link
Member

No description provided.

@robgietema robgietema requested a review from sneridagh April 5, 2023 12:42
@netlify
Copy link

netlify bot commented Apr 5, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 27745be
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/642e6ed8dfbe3f0008f7fb88

@robgietema robgietema force-pushed the querystring-search-get branch from da649a1 to 01e7433 Compare April 5, 2023 12:45
@erral
Copy link
Member

erral commented Apr 5, 2023

This change should be properly documented.

We faced not being able to use the @search endpoint passing a lot of parameters because there is a limitation on the HTTP standard about the URL length, we had to change our calls to the @querystring-search because this endpoint uses POST instead of GETs.

@cypress
Copy link

cypress bot commented Apr 5, 2023

Passing run #4831 ↗︎

0 489 20 0 Flakiness 0

Details:

Merge branch 'master' into querystring-search-get
Project: Volto Commit: 27745be342
Status: Passed Duration: 12:45 💡
Started: Apr 6, 2023 7:07 AM Ended: Apr 6, 2023 7:20 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@robgietema
Copy link
Member Author

This change should be properly documented.

We faced not being able to use the @search endpoint passing a lot of parameters because there is a limitation on the HTTP standard about the URL length, we had to change our calls to the @querystring-search because this endpoint uses POST instead of GETs.

The change is not enabled by default, so by default this will still use POST and not GET. You can opt-in to this behavior if you choose to do so. Discussion about this implementation can be found here: plone/plone.restapi#1252

@robgietema robgietema merged commit 9d767d4 into master Apr 6, 2023
@robgietema robgietema deleted the querystring-search-get branch April 6, 2023 07:35
@tisto
Copy link
Member

tisto commented Apr 6, 2023

@erral I agree that this should be properly documented. As @robgietema already mentioned, we are going to fully document this in plone.restapi at first. This should go into the Volto docs as well.

Background: we are currently running a set of extensive load tests against a new Plone 6 / Volto 16 deployment of a high-profile website in Germany which will launch in April.

Our analysis revealed that the querystring-search endpoint is responsible for a considerable amount of cache misses. Therefore we plan to switch from using POST to using GET for this installation. We will closely monitor if this approach has any downsides when we run into HTTP limits. Since most requests come from search/listing endpoints we do not expect to run into any limitations. Though, you never know. If that happens we might have to re-think our approach.

Could you elaborate on what problems you exactly ran into? This might help us to plan our next steps...

sneridagh added a commit that referenced this pull request Apr 10, 2023
* master: (326 commits)
  Make URL a literal string to fix broken link (#4667)
  Move developer guidelines to contributing #4665 (#4666)
  Update Volto contributing to align with and refer to the new Plone co… (#4634)
  Release @plone/scripts 3.0.0
  Changelog
  Added querystring search get option (#4658)
  Add current page parameter to the route in the listing and search block pagination (#4159)
  Fixed wrong localization on password reset page(#4656) (#4657)
  Release notes for 16.19.0 (#4655)
  Razzle upgrade notice in upgrade guide (#4641)
  Release generate-volto 7.0.0-alpha.3
  Update to latest Razzle - needed since #3997. This fixes the duplicated Razzles issue (#4640)
  3092 improve spellcheck (#4633)
  developer process for first time contributing (#4617)
  Trigger CI on pull_request event (#4629)
  Pining of `pydata-sphinx-theme` and `sphinx-book-theme`, CI is complaining. (#4626)
  Set sameSite in `18N_LANGUAGE` cookie (#4627)
  Update simple-git (#4546)
  DefaultView (blocks disabled): Show field name as tip on hover of label (#4598)
  Fix regexp that checks valid URLs and improve tests (#4601)
  ...
sneridagh pushed a commit that referenced this pull request Apr 11, 2023
sneridagh added a commit that referenced this pull request Apr 11, 2023
Co-authored-by: Rob Gietema <rob.gietema@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants